Thank you for your contribution @danielmarbach! We will review the pull request and get back to you soon.

@jsquire is there any reason why the TrackPublishHandlerAsActiveAsync calls invoke the On...Async methods they wrap with Task.Run? They are truly asynchronous methods, and I'm wondering why offloading is required there

@jsquire is there any reason why the TrackPublishHandlerAsActiveAsync calls invoke the On...Async methods they wrap with Task.Run? They are truly asynchronous methods, and I'm wondering why offloading is required there

The OnXXAsync methods invoke and await user-provided code in the event handler. We can't know if the user code is truly asynchronous or is doing a heavy bit of synchronous work and just returning Task.CompletedTask at the end. The Task.Run is intended to ensure that we don't block on the event handler. We track them as active so that during shutdown we can block on them and ensure that all the handlers have completed before we finish. The idea is that we don't want to have someone call CloseAsync and we return and they terminate their application with handlers pending/running.

@jsquire is there any reason why the TrackPublishHandlerAsActiveAsync calls invoke the On...Async methods they wrap with Task.Run? They are truly asynchronous methods, and I'm wondering why offloading is required there

The OnXXAsync methods invoke and await user-provided code in the event handler. We can't know if the user code is truly asynchronous or is doing a heavy bit of synchronous work and just returning Task.CompletedTask at the end. The Task.Run is intended to ensure that we don't block on the event handler. We track them as active so that during shutdown we can block on them and ensure that all the handlers have completed before we finish. The idea is that we don't want to have someone call CloseAsync and we return and they terminate their application with handlers pending/running.

Got it. It is a pity because now we offload even for true asynchronous methods. In general, if we can avoid doing Task.Run during runtime for asynchronous method that would be hugely beneficial for the runtime behavior since we wouldn't need to go through creating pressure on the threadpool for no added value.

Got it. It is a pity because now we offload even for true asynchronous methods. In general, if we can avoid doing Task.Run during runtime for asynchronous method that would be hugely beneficial for the runtime behavior since we wouldn't need to go through creating pressure on the threadpool for no added value.

Agreed, but I don't have a better approach. If you've got ideas, I'd love to hear them.

@danielmarbach: Are you good with me merging, or do you still have some things in flight?

I'm good to go. Will put the other topic into the attic of my brain. Maybe something falls out of it with the next dedusting of the attic 😁

I'm good to go. Will put the other topic into the attic of my brain. Maybe something falls out of it with the next dedusting of the attic 😁

I have some thoughts. If I understood correctly, we are mostly concerned about making sure the publishing logic is never impacted by any delegate that might be synchronously invoked. Yet there is a maximum of one delegate that can be assigned so there is no need for multiple delegates to run concurrent. We also need to make sure to wait for outstanding delegate invocations to be called while closing and due to that in the majority of cases we end up doing the task tracking and continuation attachment plus the additional task-based machinery including an offloading to the worker thread pool per invocation.

So how about simply storing a readonly WorkStruct containing the batch list, the partition id and an optional exception inside a channel and have a dedicated background task that takes item from the channel and invokes the corresponding delegates? With that the task tracking is also no longer necessary because you can simply await the completion of the background channel reader and because the channel reader is offloaded already you don't need to pay the price of offloading every delegate invocation.

I realized that would be a behavior breaking change because the event handler delegate attached would then no longer be invoked concurrently but one after another so that proposal doesn't work unless that is something you are willing to break or introduce a flag on the options

there is no need for multiple delegates to run concurrent.

That's not quite a correct statement. Partitions are independent and the delegate runs concurrently with a default of "limit to one invocation per partition" - but this is configurable. It is possible (and likely for high-throughput scenarios) to allow concurrent execution of partitions. We cap everything to the maximum concurrency option, but it's fair to say in most systems that you'll have several callbacks being executed concurrently.

So how about simply storing a readonly WorkStruct containing the batch list, the partition id and an optional exception inside a channel and have a dedicated background task that takes item from the channel and invokes the corresponding delegates?

It's an interesting thought, but I'm concerned that will create a throttle point. For scenarios where we have a lot of small events being sent frequently, especially when concurrency is dialed up, its a very real possibility that we'd end up queuing a chain of callbacks that we can't keep up with. Either the channel would have to grow unbounded, or we'd start blocking when things get full.

If we start seeing scheduling issues due to the backgrounding of handlers, our best bet is probably to consider awaiting the invocation at the call site. That would keep us with a known degree of concurrency and shift responsibility to developers - if you want your publishing to run quickly, do less in your handlers. We considered that, but instead kept the default degree of concurrency constrained to avoid pressure on the thread pool.

Maybe, ultimately, that's the way to go. We haven't seen anything crazy in testing, but we're also still pending stress runs.

The other option would be to not wrap the event invocation with Task.Run and still fire&forgot the execution. Then all true async delegates are executed concurrent without creating threadpool pressure and then have guidance showning how to offload synchronous invocation in the handler if desired. But I understand this might cross a usability threshold that makes the library hard to crasp for certain users which is probably not something you want to go for

The other option would be to not wrap the event invocation with Task.Run and still fire&forgot the execution. Then all true async delegates are executed concurrent without creating threadpool pressure and then have guidance showning how to offload synchronous invocation in the handler if desired. But I understand this might cross a usability threshold that makes the library hard to crasp for certain users which is probably not something you want to go for

It's worth considering, for sure, but you nailed the primary concern. From observations during user studies, I don't think the "sync until you await something" behavior is well understood. There seems to be a common belief that anytime you return Task, you are executing asynchronously.

@danielmarbach: Thank you for the discussion here. I've been kicking this around the last couple of days and it came up in a similar conversation. I think that we originally underestimated the value of having determinism for the timing of when the success/fail handler is invoked after the send and ensuring the order they'll fire with a single degree of concurrency.

I'm going to change the implementation to invoke the handler and await at the completion of the Send. We'll try adding guidance for "if you want to maximize throughput, don't do a lot of direct work in your handler."

Update

Changed in #27173.